-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Add client headers to Toolbox #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
# Conflicts: # packages/toolbox-core/src/toolbox_core/client.py # packages/toolbox-core/src/toolbox_core/tool.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit for next time: given the size of the PR, it would have been better to introduce the refactoring to use utility functions in a different PR first.
Co-authored-by: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>
This has been done in another PR: ca88369. I just edited the import statement. However, it's interesting that even with the wrong import in I replicated this PR locally and no tests fail.
but @anubhav756 do you have any ideas why? |
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
* chore: Add unit test coverage. * chore: Consolidate auth header creation logic Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code. * chore: Delint * chore: Delint
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
…istency (#219) * chore: Add unit test coverage. * chore: Consolidate auth header creation logic Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code. * chore(toolbox-core): Standardize on ValueError for improved error consistency This PR updates instances where generic `Exception` objects were thrown in `toolbox-core` to use `ValueError` instead. This aligns with the prevalent use of `ValueError` for argument-related issues elsewhere in the package, leading to more specific, predictable, and robust error handling for consumers of `toolbox-core`. * chore: Delint * chore: Remove duplicate test
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
* chore: Add unit test coverage. * chore: Consolidate auth header creation logic Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places. This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code. * docs: Add names to return values for better readability * chore: Improve readability * chore: Remove redundant tests * feat: Introduce identifying used authz token getters This PR adds the feature in `identify_required_authn_params` helper to determine which of the provided auth token getters are actively used to satisfy a tool's authorization requirements (as defined by the `authRequired` key in its manifest). This is a foundational step towards future validation in `ToolboxTool.add_auth_token_getters`, which will ensure no configured auth token getters remain unused, thereby preventing potential misconfigurations.
Added a
Client Headers
feature, which allows users to bind certain headers to the client. These headers will be used in every subsequent request sent through the client.add_headers
method toToolboxClient
.ToolboxClient
init.This would allow us to deploy Toolbox Server securely to various deployment environments like Cloud Run and connect using Authorization.
For more information, see Toolbox Cloud Run Authentication.
Note: I'll add the same feature in the Sync Client in a following PR.